-
Notifications
You must be signed in to change notification settings - Fork 510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Malloc for path's buffers instead of stack #5888
base: master
Are you sure you want to change the base?
Conversation
f0bc8f1
to
9e55da3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, 1 of 3 files at r2, 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @grom72 and @osalyk)
a discussion (no related file):
I understand that having stack stats generation on the same branch is convenient for you but it is not for the reviewers nor necessary.
src/core/out.c
line 192 at r3 (raw file):
} Free(log_file_pid); }
It is outrageous. This piece of code has three possible error cases and each of them prints its error message differently. Please sort it out or at least do not introduce the third one.
Code quote:
if ((log_file = os_getenv(log_file_var)) != NULL &&
log_file[0] != '\0') {
/* reserve more than enough space for a PID + '\0' */
char *log_file_pid;
log_file_pid = Malloc(PATH_MAX);
if (log_file_pid == NULL) {
fprintf(stderr, "out_init !Malloc\n");
abort();
}
size_t len = strlen(log_file);
if (len > 0 && log_file[len - 1] == '-') {
if (util_snprintf(log_file_pid, PATH_MAX, "%s%d",
log_file, getpid()) < 0) {
ERR("snprintf: %d", errno);
Free(log_file_pid);
abort();
}
log_file = log_file_pid;
}
if ((Out_fp = os_fopen(log_file, "w")) == NULL) {
char buff[UTIL_MAX_ERR_MSG];
util_strerror(errno, buff, UTIL_MAX_ERR_MSG);
fprintf(stderr, "Error (%s): %s=%s: %s\n",
log_prefix, log_file_var,
log_file, buff);
Free(log_file_pid);
abort();
}
Free(log_file_pid);
}
src/core/out.c
line 199 at r3 (raw file):
#ifdef DEBUG static char namepath[PATH_MAX];
I do not know why this one was static but assuming its value has to survive between function calls I assume it is not allocated on the stack, or is it?
Code quote:
static char namepath[PATH_MAX];
src/core/out.c
line 209 at r3 (raw file):
#ifdef DEBUG char *namepath; namepath = Malloc(PATH_MAX);
Suggestion:
char *namepath = Malloc(PATH_MAX);
src/core/out.c
line 215 at r3 (raw file):
} LOG(1, "pid %d: program: %s", getpid(), util_getexecname(namepath, PATH_MAX));
Code snippet:
Free(namepath);
src/libpmem2/auto_flush_linux.c
line 94 at r3 (raw file):
int cpu_cache = 0; domain_path = Malloc(PATH_MAX);
Suggestion:
domain_path = Malloc(PATH_MAX * sizeof(char));
src/libpmem2/deep_flush_linux.c
line 37 at r3 (raw file):
char rbuf[2]; deep_flush_path = Malloc(PATH_MAX);
Please apply consistently.
Note: Just a single space around the assignment sign (=
).
Suggestion:
deep_flush_path = Malloc(PATH_MAX * sizeof(char));
src/libpmem2/deep_flush_linux.c
line 40 at r3 (raw file):
if (deep_flush_path == NULL) { ERR("!Malloc"); ret = -1;
Suggestion:
ret = PMEM2_E_ERRNO;
src/libpmem2/deep_flush_linux.c
line 81 at r3 (raw file):
end: if (deep_flush_fd > -1)
Suggestion:
deep_flush_fd != -1
src/libpmem2/pmem2_utils_linux.c
line 47 at r3 (raw file):
spath = Malloc(PATH_MAX); if (spath == NULL) { errno = ENOMEM;
Unnecessary. Please see other comments.
src/libpmem2/pmem2_utils_linux.c
line 68 at r3 (raw file):
npath = Malloc(PATH_MAX); if (npath == NULL) { errno = ENOMEM;
.
src/libpmem2/pmem2_utils_linux.c
line 94 at r3 (raw file):
Free(spath); if (npath) Free(npath);
Free in the reverse order they were created.
Suggestion:
if (npath)
Free(npath);
if (spath)
Free(spath);
src/libpmem2/region_namespace_ndctl.c
line 40 at r3 (raw file):
No need to set errno
by yourself.
On error, these functions return NULL and set errno.
Ref: https://man7.org/linux/man-pages/man3/malloc.3.html
src/libpmem2/region_namespace_ndctl.c
line 43 at r3 (raw file):
ERR("!Malloc"); return PMEM2_E_ERRNO;
Unnecessary empty line.
src/libpmem2/region_namespace_ndctl.c
line 65 at r3 (raw file):
LOG(4, "found matching device: %s", path); end:
Suggestion:
LOG(4, "found matching device: %s", path);
end:
src/libpmem2/region_namespace_ndctl.c
line 92 at r3 (raw file):
path = Malloc(PATH_MAX); if (path == NULL) { errno = ENOMEM;
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)
a discussion (no related file):
Previously, janekmi (Jan Michalski) wrote…
I understand that having stack stats generation on the same branch is convenient for you but it is not for the reviewers nor necessary.
Shall we keep stats files up-to-date?
bd6e8de
to
de0c594
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)
src/core/out.c
line 192 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
It is outrageous. This piece of code has three possible error cases and each of them prints its error message differently. Please sort it out or at least do not introduce the third one.
Done.
src/core/out.c
line 199 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
I do not know why this one was static but assuming its value has to survive between function calls I assume it is not allocated on the stack, or is it?
Can we assume that out_init will be called once at a time?
src/core/out.c
line 209 at r3 (raw file):
#ifdef DEBUG char *namepath; namepath = Malloc(PATH_MAX);
Done.
I mean use static variable here.
src/core/out.c
line 215 at r3 (raw file):
} LOG(1, "pid %d: program: %s", getpid(), util_getexecname(namepath, PATH_MAX));
Done.
Rollback to the original solution with static.
src/libpmem2/deep_flush_linux.c
line 37 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Please apply consistently.
Note: Just a single space around the assignment sign (
=
).
Done.
src/libpmem2/deep_flush_linux.c
line 40 at r3 (raw file):
if (deep_flush_path == NULL) { ERR("!Malloc"); ret = -1;
Done.
0cf4069
to
1210c10
Compare
Codecov Report
@@ Coverage Diff @@
## master #5888 +/- ##
==========================================
- Coverage 71.00% 68.65% -2.36%
==========================================
Files 131 131
Lines 19172 19725 +553
Branches 3192 3265 +73
==========================================
- Hits 13614 13543 -71
- Misses 5558 6182 +624 |
5e632fb
to
9176a8d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, 7 of 12 files at r4, 2 of 9 files at r5, 1 of 1 files at r6, 4 of 6 files at r7, 3 of 3 files at r8, 1 of 1 files at r9.
Reviewable status: 10 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)
src/libpmem2/auto_flush_linux.c
line 94 at r3 (raw file):
int cpu_cache = 0; domain_path = Malloc(PATH_MAX);
Done.
src/libpmem2/deep_flush_linux.c
line 81 at r3 (raw file):
end: if (deep_flush_fd > -1)
It is opposite to the previous condition ....<0
src/libpmem2/pmem2_utils_linux.c
line 47 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Unnecessary. Please see other comments.
Done.
src/libpmem2/pmem2_utils_linux.c
line 68 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
.
Done.
src/libpmem2/pmem2_utils_linux.c
line 94 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Free in the reverse order they were created.
Done.
src/libpmem2/region_namespace_ndctl.c
line 40 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
No need to set
errno
by yourself.On error, these functions return NULL and set errno.
Done.
src/libpmem2/region_namespace_ndctl.c
line 43 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Unnecessary empty line.
Done.
src/libpmem2/region_namespace_ndctl.c
line 65 at r3 (raw file):
LOG(4, "found matching device: %s", path); end:
Done.
src/libpmem2/region_namespace_ndctl.c
line 92 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 10 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)
Signed-off-by: Tomasz Gromadzki <[email protected]>
9176a8d
to
9dfd9c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r10, 3 of 3 files at r11, all commit messages.
Reviewable status: 11 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 9 files at r5.
Reviewable status: 12 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)
Decrease the amount of stack memory usage by allocating the path buffer on the heap using the Malloc() function.
Requires:
Signed-off-by: Tomasz Gromadzki [email protected]
This change is